fix(worker): respect deadline in commit condition#1243
fix(worker): respect deadline in commit condition#1243
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMain loop in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant W as ScrollWorker
participant T as TxPool
participant C as Chain
rect rgba(220,235,255,0.30)
note over W: Main loop iteration (new)
W->>T: Fetch pending transactions (ev.Txs)
W->>W: processTxnSlice(ev.Txs) -- always run
W->>W: Check block deadline and tx count
alt deadline reached AND tx_count > 0
W->>C: Commit block (use current time ≤ deadline)
C-->>W: Commit result
else otherwise
W->>W: Wait / continue accumulating txs
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where the signer node commits blocks without waiting for the deadline, leading to ErrFutureBlock consensus failures under high load. The fix ensures that blocks are only committed when both the processing condition is met AND the deadline has been reached.
- Modified the commit condition to require both
shouldCommitanddeadlineReachedto be true - Added clarifying comments explaining the logic
- Updated the patch version from 5 to 6
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| miner/scroll_worker.go | Updated commit condition to respect deadline and added explanatory comments |
| params/version.go | Incremented patch version from 5 to 6 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
miner/scroll_worker.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
miner/scroll_worker.go (1)
417-422: Verify impact of removing early commits on block production latency.The fix prevents
ErrFutureBlockby only committing after the deadline, but also defers all full-block commits (the variousshouldCommitcases inprocessTxnSlice) until that deadline. Confirm this won’t harm throughput under load by reviewing those early-commit triggers (e.g. overflow, fork, L1 message logic) and monitoring block production latency metrics.
|
Failed to reproduce and test issue. |
1. Purpose or design rationale of this PR
It was reported that under high load, the signer node runs into
ErrFutureBlockconsensus failures. The root cause seems to be that onceprocessTxnSlicereturnstrue, we commit regardless of the deadline (block timestamp). The timestamp without relaxed mode is set toparent.Time + Periodwhich might be in the future.2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Bug Fixes
Chores